Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DTNN-653 Rebaseline for v7 development #261

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from

Conversation

sara-gb
Copy link
Collaborator

@sara-gb sara-gb commented Jul 18, 2024

Describe the contribution
Addresses DTNN-653. Rebaselines the repo with the necessary build structure and initial files needed for v7 development

Testing performed
Unit and build tests

Expected behavior changes
Library prototype is wiped and replaced with initial v7 files. All components have an empty init function and unit test framework.

System(s) tested on

  • Linux

Contributor Info - All information REQUIRED for consideration of pull request
Sara Garcia-Beech

@gskenned
Copy link
Contributor

Changes look good.
Built with files from
bplib Draft PR-260 Update README, tag PR-260.02.

gskenned@ip-10-1-23-64:~/repos/forks/bplib/doc/example-scripts/stand-alone
$ git log -1
commit 06749141411303be1c448503a56f1591ffbce49d (HEAD -> DTNN-132-clean-up-bplib-prototype-readme, tag: PR-260.02, origin/DTNN-132-clean-up-bplib-prototype-readme)
<truncated>

See bplib-dtnn-653.zip attached.

Build is in bplib-dtnn-653/build-bplib.
Copied all files from PR-260.02 bplib/doc/example-scripts/stand-alone to build-bplib.
Modified bplib-env-vars. The original is bplib-env-vars.0.
Ran ./bplib-test-driver in build-bp because I already had the toolchain installed. (Otherwise, run ./bplib-install-toolchain.)
This is a build test only. Successfully built bpcat for both Debug-POSIX and Release-POSIX.

gskenned@ip-10-1-23-64:~/repos/reviews
$ find -name bpcat
./bplib-dtnn-653/build-bplib/bplib-build-matrix-Debug-POSIX/app/bpcat
./bplib-dtnn-653/build-bplib/bplib-build-matrix-Release-POSIX/app/bpcat

See the logs in the tar file:

$ find -name "bplib-test-driver*.log"
./bplib-dtnn-653/build-bplib/bplib-test-driver-24211-1132-r-p.log
./bplib-dtnn-653/build-bplib/bplib-test-driver-24211-1132-d-p.log

@sara-gb sara-gb self-assigned this Aug 6, 2024
@sara-gb sara-gb marked this pull request as ready for review August 6, 2024 21:14
@gskenned
Copy link
Contributor

gskenned commented Aug 7, 2024

Great work!
I built and ran this branch with bp-cfs/develop and bpnode/develop and COSMOS. It ran well with no issues.

  1. General question: Does the github runner run as root, or with sudo root, or as a regular user?
    I ask because I found that the common rbtree and ut_functional sanity tests required setuid root to work when run by a regular user. I don't know if it's an issue or not what privileges the github runner user has. I just think we should know.

  2. Commit 01d9a08 has too many lines of commit messages.

commit 01d9a085d048b6d58b97d605c1bbd3bce32fc4e5
Author: Sara Garcia-Beech <[email protected]>
Date:   Fri Aug 2 16:01:12 2024 +0000

    DTNN-653 Fixed functional test cmake
    
    DTNN-653 Added include dirs to functional test
    
    DTNN-653 Added link libs to functional test
    
    DTNN-653 Functional test compilation mods
    
    DTNN-653 Added bplib stubs to functional test
and so on, for about 58 more lines.
  1. Where does the bplib build find utassert.h?
    In bp-cfs it is in osal/ut_assert/inc/utassert.h.
    Does the use of utassert.h depend on having OSAL?

  2. In a future version, I'd like to see the empty unit tests, like BPLib_Test in ut-functional/sanity-test.c, report that they are "unimplemented" or an "empty stub". They should still pass in the .github testing, but then we can see which tests are implemented and which are not.

@sara-gb
Copy link
Collaborator Author

sara-gb commented Aug 7, 2024

@gskenned Replying to your comments:

  1. No idea, I've never set up github runners myself
  2. Ugh, I was trying to squash those commits, thanks for noticing that. I'll try to fix it.
  3. The BPLib unit test build depends on utassert/osal, but not the library build itself. If someone wanted to just build the library, they can do that as a standalone.
  4. Once people start adding at least stub functions to their components, that's a good idea, but right now the stubs are just temporary init functions, a lot of which will probably go away, I just wanted at least something in the source files to compile

@sjonke
Copy link
Collaborator

sjonke commented Aug 7, 2024

I built and ran with cosmos, running the test suite there and it worked without issue. Ran the unit tests and those were successful as well. I looked through the changes. I did not look at absolutely everything - after a point I started skimming through them noting that the general, repeating pattern was there. It looks good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants